Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install xargo using CI dictated cargo version if available #9068

Merged
merged 12 commits into from
Mar 26, 2020

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Mar 25, 2020

Problem

The SDK install script attempts to install xargo which is required to build Rust BPF programs. The script uses whatever version of cargo is installed on the host machine to do so. in CI the version of cargo is always explicit and the machine's default toolchain is not updated, consequently the CI machines default version is very old. This old version does not support installing the latest version of xargo.

Summary of Changes

  • Use the CI version of cargo if the CI defined cargo version environment variable is defined (rust_stable.
  • cargo install now installs the latest version of a package so remove the use of cargo install-update

Fixes #

@jstarry
Copy link
Member

jstarry commented Mar 26, 2020

FYI looks like cargo install xargo will now update xargo to the latest version and will do nothing if it's already installed. This was released as part of 1.41.0: rust-lang/cargo#7560 So we can rip out cargo-update

@jackcmay
Copy link
Contributor Author

@jstarry Cool, I'll both set the stable version as the default toolchain and rip out update and replace with cargo install

@jstarry
Copy link
Member

jstarry commented Mar 26, 2020

@jackcmay, @mvines just added a note in #devops about setting the default toolchain being disruptive for devs, so we should be careful about that. Maybe we could change the command to cargo +stable to avoid that?

@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #9068 into master will increase coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #9068     +/-   ##
========================================
+ Coverage    80.4%   80.4%   +<.1%     
========================================
  Files         268     268             
  Lines       58796   58796             
========================================
+ Hits        47320   47326      +6     
+ Misses      11476   11470      -6

@jackcmay
Copy link
Contributor Author

Agreed, not going to set default toolchain

set -e
cargo install-update -i xargo
set -ex
cargo +"${rust_stable:-}" install xargo
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvines know any cool bash tricks to improve this?

set -e
cargo install-update -i xargo
set -ex
cargo +"${rust_stable:-}" install xargo
Copy link
Member

@mvines mvines Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just go with a simple approach:

Suggested change
cargo +"${rust_stable:-}" install xargo
if [[ -n $rust_stable ]]; then
cargo +"$rust_stable" install xargo
else
cargo install xargo
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shellcheck does not like this since it can't tell if rust_stable is defined (rust_stable should probably be in all caps) and would rather not have to clutter up with a spellcheck "allow" statement

@jackcmay jackcmay changed the title Recreate xargo update issue Install xargo using CI dictated cargo version if available Mar 26, 2020
@jackcmay jackcmay merged commit 30bed18 into solana-labs:master Mar 26, 2020
@jackcmay jackcmay deleted the xargo-version branch March 26, 2020 18:47
@jackcmay jackcmay added the v1.0 label Mar 26, 2020
mergify bot pushed a commit that referenced this pull request Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants